Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhancement: RateLimitFilter - Provides an exact rate limiting mechanism #794

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Chenjp
Copy link
Contributor

@Chenjp Chenjp commented Dec 6, 2024

smaller PR from #767 .
If you need exact rate limiting and can accept a small decrease in efficiency, ExactRateLimiter may be an alternative option.

you need exact rate limiting and can accept a small decrease in efficiency, ExactRateLimiter may be an alternative option.
@Chenjp Chenjp changed the title enhancemen: RateLimitFilter - Provides an exact rate limiting mechanism enhancement: RateLimitFilter - Provides an exact rate limiting mechanism Dec 6, 2024
@Chenjp
Copy link
Contributor Author

Chenjp commented Dec 16, 2024

minimize API change.

@markt-asf
Copy link
Contributor

You can't remove methods from the RateLimiter interface as it been included in a stable release.

@Chenjp
Copy link
Contributor Author

Chenjp commented Dec 17, 2024

You can't remove methods from the RateLimiter interface as it been included in a stable release.

Updated

Copy link
Contributor

@markt-asf markt-asf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR makes a very large number of unnecessary changes which makes it extremely hard to review. I have already spend multiple hours just reviewing the refactoring of TimeBucketCounter.

I have yet to decide whether I will proceed with applying those changes as a first step towards to implementing the changes proposed in this PR or whether I'll opt to close this PR and request a series of smaller PRs where each PR addresses a single concern and, potentially, breaks larger, related changes into a series of smaller, reviewable commits.

Comment on lines +175 to +186
public void periodicEvict() {
final long minBucketIndex = getBucketIndex(System.currentTimeMillis());
// assume that elapsed time of periodicEvict less than 1x bucketDuration.
// to avoid extreme case: 999999-xxx vs 1000000-xxx
final long maxBucket = minBucketIndex + 2;

final String minBucketPrefix = minBucketIndex + BUCKET_KEY_DELIMITER;
final String maxBucketPrefix = maxBucket + BUCKET_KEY_DELIMITER;

// remove obsolete items whose key are less than minBucketPrefix and maxBucketPrefix in same time.
map.keySet().removeIf(k -> k.compareTo(minBucketPrefix) < 0 && k.compareTo(maxBucketPrefix) < 0);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What problem is the change to the eviction mean to solve?

markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
Based on PR #794 by Chenjp
@markt-asf
Copy link
Contributor

It turns out the TimeBucketCounter was by far the most complex. Once that was reviewed, the rest followed quite quickly. I'm leaving this PR open as there are some changes - particularly the change to eviction - that might need to be added.

markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
Based on PR #794 by Chenjp
markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
Based on PR #794 by Chenjp
markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
markt-asf added a commit that referenced this pull request Mar 7, 2025
Based on PR #794 by Chenjp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants